Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide feedback if it takes a while to download a remote component on spin up #2226

Merged
merged 3 commits into from
Jan 18, 2024

Conversation

Archisman-Mridha
Copy link
Contributor

No description provided.

Signed-off-by: Archisman Mridha <archismanmridha12345@gmail.com>
@itowlson itowlson changed the title Fix #2109 Provide feedback if it takes a whole to download a remote component on spin up Jan 16, 2024
Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Archisman-Mridha, welcome and thanks!

I'm afraid this PR doesn't answer the issue you mentioned (unless I've misread the link), and it's not quite the way we usually do these "taking a long time" warnings anyway. What you have here would always print a message whenever an application was pulled or run from an OCI registry reference. What #2109 is looking for is to print a message for a remote component, and to print it if it takes a while. Let me drill into those two phrases a bit:

  • By a "remote component", we mean a component referenced via a remote reference. Today, the only remote references for components are HTTP references such as the one in the static-fileserver template. The emphasis on component here is because such a component could be in a local application, even a freshly created one. (It's more surprising when a local app is slow to load than when a remote app is slow to load!) This is a different code path from pulling an entire from the registry to local - see e.g. https://github.com/fermyon/spin/blob/main/crates/loader/src/http.rs.

  • By "if it takes a while", we mean we don't print "downloading / starting / whatever" every time the user does the thing. This risks the user tuning out the warning that they see all the time, so not registering it when it actually matters. Another example of this is if an app's Wasm modules are taking a long time to load. In this case we wait 1.25 seconds before displaying a "slow load" message (

    let _sloth_guard = warn_if_wasm_build_slothful();
    ). The spin_common::sloth module provides a function which manages this by running a background timer and printing only if the returned guard is still live when the timer expires.

I hope this is useful feedback - of course please let me know if the desired behaviour or the way to go about it is not clear. And thanks again for getting involved!

@Archisman-Mridha Archisman-Mridha changed the title Provide feedback if it takes a whole to download a remote component on spin up Provide feedback if it takes a while to download a remote component on spin up Jan 16, 2024
@itowlson
Copy link
Contributor

Oops. Thanks for the catch on the title @Archisman-Mridha...! embarrassed

@Archisman-Mridha Archisman-Mridha marked this pull request as draft January 16, 2024 04:08
@Archisman-Mridha
Copy link
Contributor Author

@itowlson Thanks for the quick and in-detail response! Sorry for misunderstanding the issue. I am working on it....

Steps I took to clear out my concepts

My current understanding

A spin application contains multiple WASM components. These WASM components can also be located in a remote fileserver. When we execute spin up, these WASM components are download to the local machine.

Components downloading is done by the verified_download function located at - crates/loader/src/http.rs. I just need to put a sloth guard inside verified_download.

Am I right ?

@itowlson
Copy link
Contributor

Your understanding is right. Your proposal is almost right, but there is a small nuance: we might now want to place the sloth guard inside verified_download, because then an app with more than one remote component would print the warning several times. This might be okay with clear messaging about which components are taking time; but more usually we place the guard around the entire process (of loading, downloading, etc.), so that it is printed only once. (Again, see the sloth guard around loading Wasm modules, where we print a single "loading is taking a while" rather than listing each component.)

Why do it this way? Consider a manifest with a large number of remote components. (For example, redirect is a remote component. A site might have a lot of redirects.) Printing a message for each of them would produce a lot of console spew for the user to burrow through. Worse, if each component takes, say, 1.1 seconds to load, but in total they take 11 seconds to load, we don't see the feedback at all!

So a more desirable plan is to find where all the remote components are being downloaded (e.g. in a loop or as a map of futures), which will be a place that directly or indirectly calls verified_download. Then the sloth guard can be placed around that loop or those futures or that function or whatever.

Does that help? Again, happy to discuss further or give you more specific pointers if that would be useful.

@Archisman-Mridha
Copy link
Contributor Author

Archisman-Mridha commented Jan 16, 2024

@itowlson Thanks for being supportive!

I figured it this is where all the components are loaded concurrently : crates/loader/src/local.rs, line number 97 - 103 :

// Load all components concurrently
let components = try_join_all(components.into_iter().map(|(id, c)| async move {
    self.load_component(&id, c)
        .await
        .with_context(|| format!("Failed to load component `{id}`"))
}))
.await?;

I need to put the sloth guard somewhere here. Am I right ?

PS: You can assign me to the issue. I didn't notice any fork made by the person who is currently assigned.

@itowlson
Copy link
Contributor

Yes, that looks right. Call warn_if_slothful immediately before that statement, and drop the returned guard immediately after (or, since there is nothing potentially slow after the statement, you can skip the drop - Rust will drop it automatically when it goes out of scope). Thanks!

Signed-off-by: Archisman Mridha <archismanmridha12345@gmail.com>
@Archisman-Mridha
Copy link
Contributor Author

Good morning :). I pushed a commit. Please let me know if the changes look okay or not.

Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach looks good - thanks for persevering with this! However, there is an important detail about keeping the SlothGuard alive (see comments). (This is on us - Rust has a way to warn about this kind of thing, and I forgot to set it up.) Other than that and a couple of nits it looks good to go though!

crates/oci/src/loader.rs Outdated Show resolved Hide resolved
crates/loader/src/local.rs Outdated Show resolved Hide resolved
crates/loader/src/local.rs Outdated Show resolved Hide resolved
Signed-off-by: Archisman Mridha <archismanmridha12345@gmail.com>
Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! I will merge once CI completes (barring any hiccups). Thanks for your patience and perseverance.

@itowlson
Copy link
Contributor

Oh! Could you mark the PR ready for review please @Archisman-Mridha? It's still draft and can't be merged in that state.

@Archisman-Mridha Archisman-Mridha marked this pull request as ready for review January 18, 2024 05:42
@Archisman-Mridha
Copy link
Contributor Author

Oh! Could you mark the PR ready for review please @Archisman-Mridha? It's still draft and can't be merged in that state.

Done

@itowlson itowlson merged commit be88d81 into fermyon:main Jan 18, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve experience if it takes a while to download a "remote" component on spin up
2 participants